Skip to content

Fixes #24872: Rework api authorization models#5669

Merged
fanf merged 1 commit intoNormation:branches/rudder/8.3from
VinceMacBuche:arch_24872/rework_api_authorization_models
Feb 28, 2025
Merged

Fixes #24872: Rework api authorization models#5669
fanf merged 1 commit intoNormation:branches/rudder/8.3from
VinceMacBuche:arch_24872/rework_api_authorization_models

Conversation

@VinceMacBuche
Copy link
Copy Markdown
Member

@VinceMacBuche VinceMacBuche commented May 16, 2024 •
edited by fanf
Loading

https://issues.rudder.io/issues/24872

Main changes are:

@VinceMacBuche VinceMacBuche requested a review from fanf May 16, 2024 21:44
@VinceMacBuche VinceMacBuche added the WIP Use that label for a Work In Progress PR that must not be merged yet label May 16, 2024
@VinceMacBuche VinceMacBuche marked this pull request as draft May 16, 2024 21:46
@VinceMacBuche VinceMacBuche removed the WIP Use that label for a Work In Progress PR that must not be merged yet label May 16, 2024
@VinceMacBuche
Copy link
Copy Markdown
Member Author

VinceMacBuche commented May 17, 2024

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the arch_24872/rework_api_authorization_models branch from 16e81d3 to 7703cc9 Compare May 17, 2024 10:39
@VinceMacBuche VinceMacBuche changed the base branch from branches/rudder/8.0 to master May 17, 2024 10:39
@VinceMacBuche
Copy link
Copy Markdown
Member Author

VinceMacBuche commented May 17, 2024

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the arch_24872/rework_api_authorization_models branch from 7703cc9 to 56e1117 Compare May 17, 2024 12:21
fanf
fanf reviewed May 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, seems great. I think it needs at least a third pair of eyes to match the API dispatching - cc @clarkenciel perhaps?

Comment thread .../sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/ApiDatastructures.scala Outdated
clarktsiory
clarktsiory reviewed Jun 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@clarktsiory clarktsiory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double-checked the authz in the endpoints, I found a few that could be changed/are not consistent

Comment thread ...ources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/EndpointsDefinition.scala Outdated
val (action, path) = DELETE / "campaigns" / "events" / "{id}"
val dataContainer: Option[String] = None
val dataContainer: Option[String] = None
val authz: List[AuthorizationType] = AuthorizationType.Configuration.Read :: Nil
Copy link
Copy Markdown
Contributor

@clarktsiory clarktsiory Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DeleteCampaignEvent here should require Write

Comment thread ...ources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/EndpointsDefinition.scala
Comment on lines 278 to 315
case object ReloadGroup extends GroupApi with GeneralApi with OneParam with StartsAtVersion2 with SortIndex {
val z: Int = implicitly[Line].value
val description = "Update given dynamic group node list"
val (action, path) = GET / "groups" / "{id}" / "reload"
val authz: List[AuthorizationType] = AuthorizationType.Group.Read :: Nil
}
Copy link
Copy Markdown
Contributor

@clarktsiory clarktsiory Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could change the list of nodes in the group, so is it safe with Read instead of Write + Edit ?

Comment thread ...ources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/EndpointsDefinition.scala
Comment on lines 801 to 897
case object CheckTechnique extends TechniqueApiPub with ZeroParam with StartsAtVersion16 with SortIndex {
val z: Int = implicitly[Line].value
val description = "Check if a techniques is valid yaml, with rudderc compilation, with various output (json ? yaml ?)"
val (action, path) = POST / "techniques" / "check"
val authz: List[AuthorizationType] = AuthorizationType.Technique.Write :: AuthorizationType.Technique.Edit :: Nil
}
Copy link
Copy Markdown
Contributor

@clarktsiory clarktsiory Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's just a check (in which case maybe it should be Read), or it does something else behind...
I saw it being used in Elm technique editor, shouldn't we allow them to just check the yaml ?

@fanf fanf force-pushed the arch_24872/rework_api_authorization_models branch from 56e1117 to 211b27e Compare February 19, 2025 20:34
@fanf fanf marked this pull request as ready for review February 19, 2025 20:37
@fanf
Copy link
Copy Markdown
Member

fanf commented Feb 19, 2025

Edit: ported it to Rudder 8.3

@fanf fanf mentioned this pull request Feb 19, 2025
Merged
@fanf
Copy link
Copy Markdown
Member

fanf commented Feb 19, 2025

PR updated with a new commit

clarktsiory
clarktsiory reviewed Feb 25, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@clarktsiory clarktsiory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it seems to be targeting master instead of branches/rudder/8.3, once retargeted it can be merged

@VinceMacBuche VinceMacBuche changed the base branch from master to branches/rudder/8.3 February 26, 2025 17:46
@VinceMacBuche VinceMacBuche force-pushed the arch_24872/rework_api_authorization_models branch from 7f36d1e to cc06982 Compare February 28, 2025 13:47
@Normation-Quality-Assistant
Copy link
Copy Markdown
Contributor

Normation-Quality-Assistant commented Feb 28, 2025

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5669
-- Your faithful QA
Kant merge: "Science is organized knowledge. Wisdom is organized life."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/97698/console)

@fanf
Copy link
Copy Markdown
Member

fanf commented Feb 28, 2025

OK, squash merging this PR

@fanf fanf force-pushed the arch_24872/rework_api_authorization_models branch from cc06982 to ab5cc55 Compare February 28, 2025 16:13
@fanf fanf merged commit ab5cc55 into Normation:branches/rudder/8.3 Feb 28, 2025
@clarktsiory clarktsiory mentioned this pull request May 20, 2025
Merged
@clarktsiory clarktsiory mentioned this pull request Jun 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fanf fanf fanf approved these changes

@clarktsiory clarktsiory clarktsiory approved these changes

Assignees

No one assigned

Labels

qa: Can't merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@VinceMacBuche @fanf @Normation-Quality-Assistant @clarktsiory